Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a new *public* main() method #5149

Closed
wants to merge 2 commits into from

Conversation

sirosen
Copy link
Contributor

@sirosen sirosen commented Apr 2, 2018

This is mostly compatible with the prior use of pip.main() but not exactly identical.
If the recommended mode of use for invoking pip programmatically is to call sys.executable with -m pip, then that mode of use must be supported by pip.

subprocess.check_output(...) behaves very similarly, but of course not identically, to pip.main(...) on pip<9.0.2

This is therefore a mostly compatible interface which alleviates some transitional hurdles for those using virtualenv or other 3rd party packages which depend upon pip.main.
The only packages which will remain broken are those which use pip.main from an environment in which subprocess cannot be spawned.

The new public pip.main is very clearly declared as the only public python API for pip and is noted as having been added for better backwards compatibility with known (but not supported) usage.

I'm well aware of the existing threads on this subject, and of a very similar, but rejected, PR ( #4798 ).

To me, it seems silly to say that pip will support invocation in this manner, but then very explicitly not provide the very small and very broadly compatible patch to make the lives of numerous pip consumers much easier.

@dstufft
Copy link
Member

dstufft commented Apr 2, 2018

If the recommended mode of use for invoking pip programmatically is to call sys.executable with -m pip, then that mode of use must be supported by pip.

I don't understand this statement. We do support python -m pip in pip 10, but that's entirely unrelated to pip.main().

The only packages which will remain broken are those which use pip.main from an environment in which subprocess cannot be spawned.

There is another class of breakages, anything that attempted to monkeypatch pip prior to calling pip.main() and expected that to be reflected in the version of pip that was eventually executed. Those people will still be broken, just subtly broken rather than obviously so.

That being said, I'm not opposed to this (though I'm not sure I'm for it either). I would prefer this to just be defined in pip/__init__.py if we do add it, so that we don't have an extra, needless module here. I want to think about this a little bit though, my concern is basically that it's different enough that it's going to break the expectations people had when executing pip.main(), and in subtle weird ways and whether that is better or worse than telling people to explicitly subprocess.

@sirosen
Copy link
Contributor Author

sirosen commented Apr 2, 2018

I don't understand this statement. We do support python -m pip in pip 10, but that's entirely unrelated to pip.main().

Sorry, my phrasing was poor.
I only mean to say that adding this does not significantly increase the number and kind of supported usages for pip 10, as subprocess.check_call([sys.executable, '-m', 'pip', ...]) already must be supported. Supporting a function which does exactly that is not tremendously burdensome in and of itself.

Of course the reality is more subtle. However, barring consideration for the prior meaning of pip.main, it's not a big effort to maintain a public method which does this.

my concern is basically that it's different enough that it's going to break the expectations people had when executing pip.main()

That's a fair concern, but I find this confusing when I look at the highly contentious thread, #5081.
The simplistic line of reasoning is something like: Either pip is concerned with callers of its hitherto unsupported pip.main, or it isn't. If it is, pip.main should not have been removed, but given warnings about a change in its behavior and replaced. If it isn't, nothing forbids adding pip.main as a new supported method now.

Refusing to add pip.main on the grounds that someone could have been monkey-patching it means that pip doesn't mind breaking all of those callers, but does mind making it hard for some of them to debug.

Recognizing that the vast majority of users of pip.main were probably just imitating what they saw virtualenv already doing, replacing it with a new and supported method is far better for them.
To me, when weighing the options -- and knowing that either choice is going to produce a lot of angst and anguish -- this seems to be by far the lesser of two evils.


I'm happy to move it into __init__ and fix the import-ordering linting errors. Will do that momentarily.

This is mostly compatible with the prior use of `pip.main()` but not
exactly identical.
If the recommended mode of use for invoking `pip` programmatically is to
call `sys.executable` with `-m pip`, then that mode of use *must* be
supported by `pip`.

`subprocess.check_output(...)` behaves very similarly, but of course not
identically, to `pip.main(...)` on pip<9.0.2

This is therefore a mostly compatible interface which alleviates some
transitional hurdles for those using virtualenv or other 3rd party
packages which depend upon `pip.main`.
The only packages which will remain broken are those which use
`pip.main` from an environment in which subprocess cannot be spawned.

The new public `pip.main` is very clearly declared as the *only* public
python API for pip and is noted as having been added for better
backwards compatibility with known (but not supported) usage.
@sirosen sirosen force-pushed the add-supported-mainfunc branch from 8b72454 to 9b10a2e Compare April 2, 2018 23:09
@pradyunsg
Copy link
Member

Relavant here: #5080

@pradyunsg
Copy link
Member

I feel we should expose a pip.main that does what this PR does -- with it additionally printing a message saying "you're doing it wrong, switch to subprocess.call or whatever".

And then 2 major version bumps later, make it go away for good.

@pfmoore
Copy link
Member

pfmoore commented Apr 3, 2018

If someone had proposed this back when we first announced that we were reorganising the internal APIs in pip 10, it might have had some merit, and I'd likely have supported it. But it's now too late for this to land in pip 10, so we're left with a situation where anyone using pip.main will have to rewrite their code to support pip 10, why would they then write it back to use pip.main for pip 10.1? So I think this ship has probably sailed.

I'm -1 on @pradyunsg's suggestion that we add it just to deprecate it again. The fact that it won't be in pip 10 makes that pointless IMO.

@sirosen
Copy link
Contributor Author

sirosen commented Apr 3, 2018

I disagree, I think this has value even in 10.1. Maybe not surprising, as I'm the person suggesting adding it back in.

For anyone running pip install -U pip, adding this back in 10.1 will mean that pip has "broken" them in 10.0, and then "fixed" the "bug" in 10.1.

Dropping it in v11 or later would be fine as far as I am concerned/this opinion runs.
Adding warning text like

warn("pip.main() is unsupported and will be removed in pip v11. "
     "If you want to invoke pip from within your program, see the documentation "
     "here: https://pip.pypa.io/en/latest/user_guide/#using-pip-from-your-program")

further enhances the value here.

@pfmoore
Copy link
Member

pfmoore commented Apr 3, 2018

But I'm assuming we're talking here about people with apps that depend on pip (to be clear, this is unsupported, but we're trying to provide a smoother transition for them). For those people, what will they do? Say their app supports pip <10 and pip 10.1-10.9 but not pip 10.0 or pip 11? What will their explanation be for not supporting pip 10? That there's a "bug" in that version? I'm really not in favour of any solution which allows people to claim that pip 10 is "buggy", so "fixing" that bug in 10.1 gives totally the wrong impression IMO.

It's not as if changing your code to call subprocess is that hard, and in doing so, you support all versions of pip, at no cost to the pip developers or your users.

@RonnyPfannschmidt
Copy link
Contributor

fro mmy pov this one is worse, since it makes code thats potentially half-working, as in the bits that call main will magically keep working while others break hard, its better to break things whole than to leave a minefield

@sirosen
Copy link
Contributor Author

sirosen commented Apr 3, 2018

For those people, what will they do? Say their app supports pip <10 and pip 10.1-10.9 but not pip 10.0 or pip 11? What will their explanation be for not supporting pip 10? That there's a "bug" in that version? I'm really not in favour of any solution which allows people to claim that pip 10 is "buggy", so "fixing" that bug in 10.1 gives totally the wrong impression IMO.

Anyone who's going to complain about pip 10.0 and 9.0.2 is going to complain about 9.0.2 and 10+
I don't know that conversations that people have about this should be of paramount importance, relative to reducing the total number of broken projects in the world at a given time.

To me, #5081 demonstrates that people have already made up their minds to be angry about pip "breaking" in a point release, and to continue being angry that the "breakage" persists into 10.x

to be clear, this is unsupported, but we're trying to provide a smoother transition for them

Yes, this whole measure is just a courtesy.

If we're concerned with public perception of pip, taking some measures to at least cope with those real and extant packages which use pip.main seems better than not taking any action at all.

It's not as if changing your code to call subprocess is that hard, and in doing so, you support all versions of pip, at no cost to the pip developers or your users.

Yes, and this is a strong argument in favor of having a warning and marking this as deprecated (rather than, as initially proposed, a new and publicly supported feature).
Invoking in a subprocess works on all versions and is safe, even if you have v9.0.2 and imported requests.


This could even be added as officially unsupported with the warning, and removing it after some shorter deprecation cycle than waiting for pip 11. It can be added in 10.1 and removed in 10.2, so long as those are spaced out enough to let people who were doing it wrong get a warning and upgrade to the right thing.

Even the extreme version of this, in which pip.main is a method which raises a error with a tidy message is cleaner and better for the community. That's different enough to probably merit a separate PR and thread of discussion though. To throw the idea out there in full though,

RuntimeError(
    "pip.main() is unsupported and should not be used. "
    "If you want to invoke pip from within your program, see the documentation "
    "here: https://pip.pypa.io/en/latest/user_guide/#using-pip-from-your-program\n"
    "You may also find that pip<10 works for this purpose, but the pip maintainers "
    "cannot and do not support such usage.")

@pradyunsg
Copy link
Member

If we add a warning, it's probably gonna show up to an end user which means they might go ahead and report it somewhere. If we don't add anything, their code breaks and they get a little annoyed and report it somewhere and pin it down to a lower version of pip.

Both of these are acceptable to me. Paul's right, pip 10.0 betas are out and we should not make non-bugfix changes now... Making this, a discussion for a post pip 10.0 for me.

@pradyunsg pradyunsg added state: needs discussion This needs some more discussion C: public api Public API stuff type: enhancement Improvements to functionality and removed state: needs discussion This needs some more discussion labels Apr 4, 2018
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Apr 15, 2018
@pradyunsg
Copy link
Member

pip 10.0 is out. Thinking a bit about pip 10.1, what @pfmoore says - about people viewing pip 10.0 as buggy - resonates with my thoughts on this now.

I'm a -1 on this change as is - it's subtly different when it breaks things + this wasn't supported in the first place. The fix/workaround/whatever-you-call-it of replacing with a subprocess call is likely less painful overall than this change.

As for introducing a pip.main with a raise only, do file a new issue (or PR if you want) -- I like that idea more.

@pradyunsg
Copy link
Member

Closing in favor of #5254.

@labrys
Copy link

labrys commented Jul 28, 2018

@pradyunsg Since the subprocess method is supported, what's the harm in making pip.main use the officially sanctioned method. You can even add a warning to it to not call pip.main directly, but at least this would fix packages that already use pip.main and may no longer be maintained. I am definitely +1 on including this.

@pfmoore
Copy link
Member

pfmoore commented Jul 28, 2018

@labrys this simply isn't going to happen. It's all been discussed multiple times, and re-opening the discussion won't change the result, it will simply annoy people. Please let this drop.

@pypa pypa locked as resolved and limited conversation to collaborators Jul 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C: public api Public API stuff needs rebase or merge PR has conflicts with current master type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants